[COLLECTIONS-776] Fix expiration bypass in PassiveExpiringMap when wrapped in SynchronizedMap#694
[COLLECTIONS-776] Fix expiration bypass in PassiveExpiringMap when wrapped in SynchronizedMap#694s8sankalp wants to merge 8 commits into
Conversation
|
-1 since applying only the |
…ion to satisfy TDD
garydgregory
changed the title
garydgregory
left a comment
garydgregory
left a comment
There was a problem hiding this comment.
Hello @s8sankalp
Thank you for the PR.
There is untested code in this PR. To see what's missing:
- Run:
mvn clean verify site - Open the site in:
target/site/index.html - Navigate to the JaCoCo report
You'll see in red what's missing, for example, null inputs on the removeAll() and retainAll() methods in the new classes.
In unit tests, this is an anti-pattern unless you are specifically testing for size() implementation instead of state:
assertEquals(0, entrySet.size());
Update or add:
assertTrue(entrySet.isEmpty());
You don't need blanks lines all over the place since you nicely document the steps with // comments.
…over null removeAll/retainAll
garydgregory
left a comment
garydgregory
left a comment
There was a problem hiding this comment.
Hello @s8sankalp
Why does PassiveExpiringMap.EntrySet.iterator() call PassiveExpiringMap.removeAllExpired(long) and PassiveExpiringMap.KeySet.iterator() does not?
Isn't that call either superfluous in the former or missing in the later?
Commenting out the call of PassiveExpiringMap.removeAllExpired(long) in PassiveExpiringMap.EntrySet.iterator() doesn't cause any test to fail.
This tells me at lease one test is missing.
Please check.
The call to removeAllExpired() is not needed in KeySet.iterator() because both the key set and values iterators delegate directly to entrySet().iterator(), which already triggers the eviction check. Conversely, the eviction call in EntrySet.iterator() is necessary to support cached views. If a client caches a collection view (like the entry set) and waits for entries to expire before obtaining an iterator, the iterator must run the eviction check to avoid returning those expired elements. To verify this behavior and prevent future regressions, I have added a new unit test asserting that iterators created from cached views after expiration do not return any elements; this test now correctly fails if the eviction call in EntrySet.iterator() is removed. |
|
Hi @garydgregory , Just wanted to check in on this one. I replied above about why removeAllExpired() is needed in EntrySet.iterator() but not in KeySet.iterator(), and I added a new unit test to cover the cached view eviction case you pointed out. Let me know if that clears things up or if you'd like me to change anything else. |
|
@s8sankalp |
|
@garydgregory |
There was a problem hiding this comment.
Pull request overview
This PR addresses a correctness gap in PassiveExpiringMap’s passive-eviction behavior when the map is wrapped by Collections.synchronizedMap(), where cached collection views (entrySet(), keySet(), values()) could bypass view-level expiration triggers and allow expired entries to linger indefinitely.
Changes:
- Introduces custom
EntrySet,KeySet, andValuesCollectionview wrappers (and iterators) to ensure view operations trigger expiration checks and iterator removals clean upexpirationMap. - Adds regression tests covering synchronized-map view caching and iterator-triggered expiration.
- Records the fix in
changes.xmlunder COLLECTIONS-776.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/main/java/org/apache/commons/collections4/map/PassiveExpiringMap.java | Replaces raw map views with custom view decorators/iterators to prevent expiration bypass and to propagate iterator removals to expirationMap. |
| src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java | Adds regression tests for synchronized-map view caching and iterator-triggered expiration; adds tests around view removals and null inputs. |
| src/changes/changes.xml | Adds a changelog entry for COLLECTIONS-776. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Override | ||
| public boolean removeAll(final Collection<?> coll) { | ||
| if (coll == null) { | ||
| return false; | ||
| } |
| @Override | ||
| public boolean retainAll(final Collection<?> coll) { | ||
| if (coll == null) { | ||
| return false; | ||
| } |
| @Override | ||
| public boolean removeAll(final Collection<?> coll) { | ||
| if (coll == null) { | ||
| return false; | ||
| } |
| @Override | ||
| public boolean retainAll(final Collection<?> coll) { | ||
| if (coll == null) { | ||
| return false; | ||
| } |
| @Override | ||
| public boolean removeAll(final Collection<?> coll) { | ||
| if (coll == null) { | ||
| return false; | ||
| } |
| @Override | ||
| public boolean retainAll(final Collection<?> coll) { | ||
| if (coll == null) { | ||
| return false; | ||
| } |
| // entrySet | ||
| assertFalse(map.entrySet().removeAll(null)); | ||
| assertFalse(map.entrySet().retainAll(null)); | ||
| // keySet | ||
| assertFalse(map.keySet().removeAll(null)); | ||
| assertFalse(map.keySet().retainAll(null)); | ||
| // values | ||
| assertFalse(map.values().removeAll(null)); | ||
| assertFalse(map.values().retainAll(null)); |
| @Test | ||
| void testCollectionViewRemoval() { | ||
| final PassiveExpiringMap<String, String> map = new PassiveExpiringMap<>(10000L); | ||
| map.put("a", "b"); | ||
| map.put("c", "d"); | ||
| map.put("e", "f"); | ||
| // Remove via entrySet iterator | ||
| final Iterator<Map.Entry<String, String>> entryIter = map.entrySet().iterator(); | ||
| assertTrue(entryIter.hasNext()); | ||
| final Map.Entry<String, String> entry = entryIter.next(); | ||
| final String removedKey = entry.getKey(); | ||
| entryIter.remove(); | ||
| assertFalse(map.containsKey(removedKey)); | ||
| // Remove via keySet iterator | ||
| final Iterator<String> keyIter = map.keySet().iterator(); | ||
| assertTrue(keyIter.hasNext()); | ||
| final String key = keyIter.next(); | ||
| keyIter.remove(); | ||
| assertFalse(map.containsKey(key)); | ||
| // Remove via values iterator | ||
| final Iterator<String> valIter = map.values().iterator(); | ||
| assertTrue(valIter.hasNext()); | ||
| final String val = valIter.next(); | ||
| valIter.remove(); | ||
| assertFalse(map.containsValue(val)); | ||
| } |
…semantics and test expirationMap cleanup
|
Hi @garydgregory , Thanks for the review! I've addressed the feedback in the latest update:
The changes have been pushed to the PR branch for review. |
Description
When
PassiveExpiringMapis decorated withCollections.synchronizedMap(), calling view methods likeentrySet(),keySet(), orvalues()returns cached view decorators managed bySynchronizedMap. This bypassesPassiveExpiringMap's view methods and avoids triggering the passive eviction logicremoveAllExpired()during iteration, resulting in expired entries remaining in the map indefinitely.This PR introduces custom collection views (
EntrySet,KeySet,ValuesCollection) and their respective iterators to:removeAllExpired()is triggered on all read and write collection view operations.remove()actions to the map's internalexpirationMap.remove()andremoveAll()for the set views.Verification
testCollectionsSynchronizedMapExpiration()verifying correct passive eviction when iterating/accessing collection views of a synchronizedPassiveExpiringMap.testCollectionViewRemoval()verifying correct cleanup ofexpirationMapwhen elements are removed via view iterators.